Claude finished @Tuanlinh12312's task in 3m 40s —— View job

Code Review: PR #2765 — Integrate riscv64im-unknown-openvm-elf target into build system

• Gather PR diff and understand scope of changes
• Review crates/toolchain/build/src/lib.rs — target switch and build-std changes
• Review crates/toolchain/openvm/src/lib.rs — entry macro changes
• Review crates/toolchain/openvm/src/pal_abi.rs — new PAL ABI bridge
• Review crates/toolchain/platform/Cargo.toml — cfg gate update
• Review guest program no_main cfg gate changes (spot-check)
• Check for missed target_os = "none" references across codebase
• Post comprehensive review

Overview

This PR switches the OpenVM guest target from a custom JSON spec (riscv64im-openvm-none-elf) to a built-in Rust target (riscv64im-unknown-openvm-elf with target_os = "openvm"), adds the PAL ABI bridge for std support, updates the entry! macro, and mechanically updates no_main cfg gates across ~100 guest programs. The scope is well-organized and the mechanical changes are consistent.

Issues

1. Potential buffer overflow in sys_rand — verify ABI contract

crates/toolchain/openvm/src/pal_abi.rs:28-31

pub unsafe extern "C" fn sys_rand(recv_buf: *mut u32, words: usize) {
    hint_random(words);
    hint_buffer_chunked(recv_buf as *mut u8, words);
}

hint_random and hint_buffer_chunked both operate in 8-byte (dword) units (see extensions/riscv/guest/src/io.rs:40). The parameter name words with a *mut u32 buffer type suggests 4-byte word semantics. If the Rust fork's PAL calls sys_rand(&mut buf, buf.len()) where buf: [u32; N], this would write N * 8 bytes into an N * 4-byte buffer — a 2x overwrite.

Please verify that the PAL ABI contract matches: if words is intended to mean u64 dwords, the function signature is misleading. If it means u32 words, the hint_random/hint_buffer_chunked calls need adjustment (e.g., (words + 1) / 2).

2. Remaining target_os = "none" cfg gates in Cargo.toml files not updated

Several Cargo.toml files still use target_os = "none" which will be incorrect for the new target_os = "openvm":

Guest-only deps that will be missed (gate evaluates to false for openvm target):

• guest-libs/keccak256/Cargo.toml:16 — cfg(all(target_arch = "riscv64", target_os = "none")) gates spin dependency

Host-only deps that will leak into guest builds (gate evaluates to true for openvm target):

• extensions/pairing/guest/Cargo.toml:29 — cfg(not(target_os = "none")) gates num-bigint, num-traits, lazy_static
• extensions/algebra/guest/Cargo.toml:19 — cfg(not(target_os = "none")) gates num-bigint, halo2curves-axiom
• guest-libs/p256/Cargo.toml:77, guest-libs/k256/Cargo.toml:29, guest-libs/pairing/Cargo.toml:29
• crates/toolchain/tests/Cargo.toml:33
• extensions/ecc/tests/programs/Cargo.toml:39

These should be updated to use any(target_os = "none", target_os = "openvm") (or not(any(...)) for host-only gates). If these are intentionally deferred to a follow-up PR, please note that in the description.

Fix this →

3. Stale JSON target spec file not removed

crates/toolchain/build/riscv64im-openvm-none-elf.json — This file is no longer used since the target is now built-in, but it still exists. Should be deleted to avoid confusion.

4. Stale RUST_TARGET_PATH and rustc_target_spec_dir()

crates/toolchain/build/src/lib.rs:25-28, 287 — The docstring still says "Directory containing the target JSON" and RUST_TARGET_PATH is still set in cargo_command(). With a built-in target, this env var and the function are unused. Consider removing them (or at least updating the doc).

Suggestions (non-blocking)

5. Linked toolchain detection heuristic is fragile

crates/toolchain/build/src/lib.rs:404

let is_linked_toolchain = !toolchain_name.contains("nightly") 
    && !toolchain_name.contains("stable") 
    && !toolchain_name.contains("beta");

This negative heuristic could misclassify custom toolchain names that happen to contain "nightly" (e.g., my-nightly-fork) or miss linked toolchains with those substrings. A more robust approach would be to check rustup toolchain list output for the (linked) suffix, or to attempt rustup component list and handle the failure gracefully rather than guessing.

6. cargo_command public API breaking change

crates/toolchain/build/src/lib.rs:254 — The with_std: bool parameter was added to the public cargo_command function, which is re-exported by openvm-sdk. This is a breaking change for any external callers. Not a problem if this branch is not yet released, but worth noting for semver if it is.

7. exit_code::SUCCESS is defined but unused

crates/toolchain/openvm/src/pal_abi.rs:11 — Minor: SUCCESS = 0 is never referenced. Remove it or mark with #[allow(dead_code)] if it's part of the ABI contract.

8. sys_write diverges on unsupported FDs without annotation

crates/toolchain/openvm/src/pal_abi.rs:64-73 — The function signature returns () but terminates the VM for non-stdout/stderr FDs. This is correct zkVM behavior, but the caller (std) expects it to return. If the Rust fork's PAL contract allows panic/abort here, this is fine; just verify it matches.

Positive observations

• The mechanical no_main cfg gate update across 100 guest files is comprehensive — zero old-style cfg_attr(target_os = "none", no_main) references remain in .rs files.
• The entry! macro simplification (removing the std-feature conditional split) is clean — with target_os = "openvm", openvm_intrinsics is the single controlling axis.
• The with_std build-std integration is minimal and well-placed.
• The Cargo.lock changes are just transitive itertools version bumps — no concern.

Summary: The core changes are sound. The main concern is issue #2 — the Cargo.toml cfg gates for target_os = "none" in several crates were not updated, which could cause incorrect dependency resolution when building with the new target. Issue #1 (potential sys_rand buffer overflow) should be verified against the Rust fork's PAL contract.

Claude finished @Tuanlinh12312's task in 3m 31s —— View job

Code Review: PR #2765 — Integrate riscv64im-unknown-openvm-elf target into build system

• Gather PR diff and understand scope of changes
• Review crates/toolchain/build/src/lib.rs — target switch and build-std changes
• Review crates/toolchain/openvm/src/lib.rs — entry macro changes
• Review crates/toolchain/openvm/src/pal_abi.rs — new PAL ABI bridge
• Review crates/toolchain/platform/Cargo.toml — cfg gate update
• Check for missed target_os = "none" references in Cargo.toml files
• Review guest program no_main cfg gate changes (spot-check)
• Check against develop-v2.1.0-rv64 base for reference implementation
• Post comprehensive review

Overview

This PR switches the OpenVM guest target from a custom JSON spec (riscv64im-openvm-none-elf, target_os = "none") to a built-in Rust target (riscv64im-unknown-openvm-elf, target_os = "openvm"), adds the PAL ABI bridge for std support, updates the entry! macro, and mechanically updates no_main cfg gates across ~100 guest programs. The mechanical .rs changes are comprehensive — zero old-style cfg_attr(target_os = "none", no_main) references remain.

Issues

1. [must-fix] Reviewer request: pal_abi.rs should be based on the develop v2 rc1 implementation

Per @shuklaayush's review comment, this file should use the develop v2 rc1 implementation as the base and adapt only the RV64-specific changes. This minimizes the diff and ensures consistency with the reviewed/merged implementation. The current version appears to be independently written with different patterns (e.g., different exit code values, different error handling approaches).

2. [bug] Potential buffer overflow in sys_rand

crates/toolchain/openvm/src/pal_abi.rs:71-73

pub unsafe extern "C" fn sys_rand(recv_buf: *mut u32, words: usize) {
    hint_random(words);
    hint_buffer_chunked(recv_buf as *mut u8, words);
}

hint_random(len) generates len random u64 values (8 bytes each), and hint_buffer_chunked(ptr, num_dwords) reads num_dwords dwords (8 bytes each) — see extensions/riscv/guest/src/io.rs:40-46. The parameter name words with a *mut u32 buffer type suggests 4-byte word semantics. If the PAL contract passes buf.len() where buf: [u32; N], this writes N * 8 bytes into an N * 4-byte buffer — a 2x overwrite.

Verify the Rust fork's PAL ABI contract. If words means u32 words, the calls need (words + 1) / 2 dwords. If it means u64 dwords, the function signature is misleading.

3. [bug] Remaining target_os = "none" cfg gates in Cargo.toml files not updated

Several Cargo.toml files still use target_os = "none" which will resolve incorrectly for the new target_os = "openvm":

Guest-only deps that will be missed (gate evaluates to false for openvm target):

• guest-libs/keccak256/Cargo.toml:16 — cfg(all(target_arch = "riscv64", target_os = "none")) gates spin dependency

Host-only deps that will leak into guest builds (gate evaluates to true for openvm target):

• extensions/pairing/guest/Cargo.toml:29,35 — cfg(not(target_os = "none")) gates num-bigint, num-traits, lazy_static
• extensions/algebra/guest/Cargo.toml:19 — cfg(not(target_os = "none")) gates num-bigint, halo2curves-axiom
• guest-libs/p256/Cargo.toml:77, guest-libs/k256/Cargo.toml:29, guest-libs/pairing/Cargo.toml:29
• crates/toolchain/tests/Cargo.toml:33
• extensions/ecc/tests/programs/Cargo.toml:39

These should be updated to use any(target_os = "none", target_os = "openvm") (or not(any(...)) for host-only gates). If intentionally deferred, note it in the description.

Fix this →

4. [cleanup] Stale JSON target spec file not removed

crates/toolchain/build/riscv64im-openvm-none-elf.json still exists. Since the target is now built-in, this file is dead code and should be deleted to avoid confusion.

5. [cleanup] Stale RUST_TARGET_PATH and rustc_target_spec_dir()

crates/toolchain/build/src/lib.rs:25-28,287 — The docstring still says "Directory containing the target JSON" and RUST_TARGET_PATH is still set in cargo_command(). With a built-in target, this env var and function are no longer needed. Consider removing them or updating the doc.

Suggestions (non-blocking)

6. Linked toolchain detection heuristic is fragile

crates/toolchain/build/src/lib.rs:404

let is_linked_toolchain = !toolchain_name.contains("nightly") 
    && !toolchain_name.contains("stable") 
    && !toolchain_name.contains("beta");

This negative heuristic could misclassify custom toolchain names containing "nightly" (e.g., my-nightly-fork). A more robust approach: attempt rustup component list --toolchain <name> and handle failure gracefully, or check rustup toolchain list output for the (linked) suffix.

7. cargo_command public API breaking change

crates/toolchain/build/src/lib.rs:254 — The with_std: bool parameter was added to the public cargo_command function, which is re-exported via openvm-sdk (see crates/sdk/src/lib.rs:18). This is a breaking change for external callers. Consider whether this should be a GuestOptions field instead, or if the breaking change is acceptable for this branch.

8. sys_cycle_count and other TODO stubs

crates/toolchain/openvm/src/pal_abi.rs:96-99 — Several functions (sys_cycle_count, sys_read, sys_read_words, sys_getenv, sys_argc, sys_argv) print TODO messages and terminate. These are reasonable stubs, but note that sys_getenv returns 0 instead of usize::MAX (the documented "not set" return value per its own doc comment at line 175), which means callers will interpret "not set" as "set with 0-byte value" and try to read the value.

Positive observations

• The mechanical no_main cfg gate update across all guest .rs files is comprehensive — zero old-style references remain.
• The entry! macro simplification (removing the std-feature conditional split, using openvm_intrinsics as the single axis) is clean and correct.
• The with_std build-std integration is minimal and well-placed.
• No references to the old target name riscv64im-openvm-none-elf remain in .rs or .toml files.

Summary: The core approach is sound. The blocking items are: (1) rebasing pal_abi.rs on the develop v2 rc1 implementation per reviewer request, (2) verifying the sys_rand buffer size semantics, and (3) updating the remaining target_os = "none" cfg gates in Cargo.toml files to avoid incorrect dependency resolution with the new target.

Note: cells_used metrics omitted because CUDA tracegen does not expose unpadded trace heights.

Commit: e8e2e8f

Benchmark Workflow